Fix failed downloads being marked as downloaded, add rip repair - #1023
Open
sjbrownrigg wants to merge 4 commits into
Open
Fix failed downloads being marked as downloaded, add rip repair#1023sjbrownrigg wants to merge 4 commits into
sjbrownrigg wants to merge 4 commits into
Conversation
Track.download() retried once and, on a second failure, logged the track to the failed-downloads database but then returned normally. Media.rip() runs preprocess -> download -> postprocess unconditionally, so postprocess() went on to call db.set_downloaded(), permanently marking a track that had never downloaded. Every later run's resolve() then saw it in the downloads database and skipped it, so the track could never be recovered -- including across restarts. Raise TrackDownloadFailedError instead, so Media.rip() stops before postprocess() and set_downloaded() is never reached for a track that actually failed. Also delete the partial file. Downloads always restart from byte 0 (BasicDownloadable opens the path "wb" and sends no Range header), so a partial is never resumed -- it just sits in the library as a truncated file that has valid tags and the right size, plays in any client, and fails to decode partway through. Album.download() and Playlist.download() already wrap each track.rip() in its own try/except and continue, so raising cannot abort a whole album or playlist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 78d76be)
PendingTrack.resolve() had four failure paths and only one of them recorded anything. If the metadata request failed, if building the metadata raised, or if fetching the downloadable URL failed, it logged a line and returned None. Nothing else happened: no file was written, no downloads-database row was added, and nothing went into the failed database. The track simply went missing from the album, and because it was never recorded as failed, `rip repair` could not find it either. PendingSingle.resolve() had the same gap on three of its paths. Call db.set_failed() on all of them so a resolve-time failure is retryable rather than invisible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit bcebf57)
The failed-downloads database was write-only: entries went in and nothing ever read them back. config.toml has always told users that "`rip repair` can be called to retry the downloads", but no such command existed. Add it. It reads the failed database, retries each item through the normal resolve/rip pipeline, and clears the ones that succeed, leaving the rest logged for a future run. Success is determined by membership in the downloads database rather than by diffing the failed database before and after, because nothing in the pipeline removes rows from the failed database -- a before/after diff would always report zero repaired. Checking the downloads database is sound because set_downloaded() is only reached from postprocess(), which a failed track no longer gets to. Failed items are cleared from the downloads database before retrying, so a repair still works on a database left inconsistent by an older version. Repaired tracks are placed in their album's folder, since a failed item is usually one track missing from an otherwise complete album, and retrying resolves it as a single. --flat restores the old behaviour of downloading it to the root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 6c54ab0)
berettavexee
pushed a commit
to berettavexee/streamrip
that referenced
this pull request
Aug 18, 2026
Cherry-picks the parts of upstream nathom#1023 the fork still needs. The core of that PR -- a failed download recorded as downloaded -- is already fixed here by a different route: download() raises after two attempts, so postprocess() and its set_downloaded() are never reached. The PR's download() rework is therefore dropped; the fork's version retries and runs an integrity check on top. What was missing: - resolve() logged six failure paths without recording them. A track dying there produces no file and no downloads row either, so it left no trace at all: it silently went missing from its album with nothing for a retry to find. Both PendingTrack and PendingSingle now call set_failed() on every exception path. - a failed download left its truncated file at the final path, in the library among the good ones. It is now removed; a cleanup error is logged rather than masking the download error being raised. - a single whose download failed kept its title in the progress header for the rest of the run, since postprocess() never ran to clear it. `rip repair` retries everything in the failed-downloads database and clears what succeeds, relying on the invariant above to tell success from failure. Repaired tracks land in their album folder by default (--flat opts out), since a repaired track is nearly always one missing from an otherwise complete album. Stale downloads rows are cleared first, or resolve() would skip the retry as already-downloaded. db.remove() carried a "NOT TESTED" warning and repair depends on it, so it now has coverage on the Failed table, including the no-op case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Repair re-added each failed track by id, which resolves as a single.
That builds album metadata from the track response -- and on Tidal a
track response carries only the album's id, title and cover:
album object keys: ['cover', 'id', 'title', 'vibrantColor', 'videoCover']
No track count, so tracktotal falls back to 1, and the artist is the
track's artists rather than the album artist. Where folder_format uses
{albumartist} or {tracktotal}, repaired tracks therefore land in a
different folder from the album they belong to. One repair of a real
library produced 18 such folders holding 107 files.
Look up each failed track's album and retry through that instead. The
metadata is then the album's own, so the folder, disc subfolders and
cover art are right, and nothing extra is downloaded: tracks already in
the downloads database are skipped. Albums are de-duplicated, so several
failures from one album cost one retry rather than several.
Anything whose album cannot be determined is still retried on its own.
berettavexee
pushed a commit
to berettavexee/streamrip
that referenced
this pull request
Aug 20, 2026
types-click and types-Pillow were stubs for a type checker this project does not have — no mypy dependency, no typing step in CI — and both sat on obsolete majors, click 7 stubs against the click 8 actually used. vulture had the opposite problem: its configuration was committed ([tool.vulture] plus a curated whitelist) but the tool itself was never declared, so poetry install did not provide it and it only lived in whichever virtualenv someone had pip-installed it into. Predictably the whitelist had drifted — _album_cache and _album_tasks were dropped when _TaskCache was encapsulated, while _TaskCache.has_pending and the repair command added by nathom#1023 were missing from it. Declare vulture, resync the whitelist, and add a Dead code workflow running the same `poetry run vulture` as locally. Move the dev dependencies to [tool.poetry.group.dev.dependencies] while here: the old form is deprecated in Poetry 2.x and warns on every invocation, and the group name is what lets the new workflow install dev deps alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
When a track fails to download twice, it gets logged to the failed-downloads database — and then marked as successfully downloaded.
Track.download()retried once and, on a second failure, calleddb.set_failed(...)but then returned normally.Media.rip()runspreprocess → download → postprocessunconditionally, sopostprocess()went on to calldb.set_downloaded(...). From that point on,PendingTrack.resolve()/PendingSingle.resolve()see the track in the downloads database and skip it, permanently. Re-running the same URL doesn't help, and it survives restarts. The only recovery is to find the missing tracks by hand.I hit this on a real library. Of 629 rows in
downloads.db, 18 were also present infailed_downloads.db— every single failed track had been recorded as downloaded.The failures also leave truncated files behind. Because downloads always restart from byte 0 (
BasicDownloadableopens the path"wb"and sends noRangeheader), a partial is never resumed — it just stays in the library as a file with correct tags and a plausible size that fails to decode partway through. I found 8 of these, silently unplayable past the truncation point.There's a second, quieter version of the same bug:
PendingTrack.resolve()has four failure paths and only one of them recorded anything. If the metadata request failed, if building metadata raised, or if fetching the downloadable URL failed, it logged a line and returnedNone— no file, no downloads row, nothing in the failed database. The track just went missing from the album with no trace anywhere.PendingSingle.resolve()had the same gap on three paths.Finally,
config.tomlhas always said "Then,rip repaircan be called to retry the downloads", but no such command existed. The failed-downloads database was write-only — nothing ever read it back.What this changes
Don't mark failed downloads as downloaded— raiseTrackDownloadFailedErroron persistent failure soMedia.rip()stops beforepostprocess()andset_downloaded()is never reached for a track that failed. Also delete the partial file, since it can never be resumed.Album.download()andPlaylist.download()already wrap eachtrack.rip()in its own try/except and continue, so raising cannot abort a whole album or playlist.Record tracks that fail while resolving— callset_failed()on the resolve paths that previously returnedNonesilently, so those failures are retryable instead of invisible.Add rip repair to retry failed downloads— implements the commandconfig.tomlalready documents. It reads the failed database, retries each item through the normal pipeline, and clears the ones that succeed.Two details worth flagging for review:
set_downloaded()is now only reachable frompostprocess(), which a failed track never gets to.--flatrestores the old root-folder behaviour.Retrying through the album
Repair originally re-added each failed track by id, which resolves as a single. That builds album metadata from the track response — and on Tidal a track response carries almost nothing about its album:
No track count, so
tracktotalfalls back to 1, and the artist is the track's artists rather than the album artist. Wherefolder_formatuses{albumartist}or{tracktotal}, repaired tracks therefore land somewhere other than the album they belong to:Running it against a real library produced 18 such folders holding 107 files.
It now looks up each failed track's album and retries through that. The metadata is then the album's own, so the folder, disc subfolders and cover art are all correct, and nothing extra is downloaded — tracks already in the downloads database are skipped, so only the missing ones are fetched. Albums are de-duplicated, so several failures from one album cost one retry rather than several. Anything whose album cannot be determined is still retried on its own.
Testing
Track.rip()against a downloadable that writes a partial file then raises, with temporary databases: confirmed it raises, logs to the failed database, does not mark downloaded, removes the partial, and leaves no stale progress-bar title.rip repairagainst a real Qobuz account and recovered all 18 stuck tracks. Roughly half the large files hit genuineIncompleteReaderrors along the way, which made a good test: on every pass the downloaded and failed databases stayed disjoint. Under the old code those would have become permanently lost tracks.Happy to split this further or adjust anything.